Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS: Remove p tags from image caption paragraphs #15366

Merged

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented May 1, 2019

This PR uses the already existing rootTagsToEliminate on RichText to remove the enclosing p tags for paragraphs in the iOS image caption field.

Gutenberg mobile side PR: wordpress-mobile/gutenberg-mobile#945

IMG_1669

<!-- wp:image -->
<figure class="wp-block-image">
<img src="https://cldup.com/cXyG__fTLN.jpg" alt=""/>
  <figcaption>Hello<br>World</figcaption>
</figure>
<!-- /wp:image -->

To test iOS:

  • Run the example app.
  • Select the image block with image.
  • Write a caption and add multiple paragraphs (with enter).
  • Switch to HTML mode.
  • Check that the caption content has br for each new line and no p tags.

@mchowning could you also check that this doesn't break Android caption field?
Thank you! 🙏

@etoledom etoledom added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label May 1, 2019
@etoledom etoledom requested a review from SergioEstevao May 1, 2019 08:48
@etoledom etoledom self-assigned this May 1, 2019
@etoledom etoledom marked this pull request as ready for review May 1, 2019 08:52
@mchowning
Copy link
Contributor

check that this doesn't break Android caption field

I don't see any issues on Android. 👍 Confirmed on both (1) a clean checkout of this PR's gutenberg and gutenberg-mobile branches; and (2) a merge of these branches with the center alignment changes in #15257.

Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some doubts about this and went to check what the web version is doing.

So for them when an Enter and no onSplit prop is defined they add a \n to the content. Then this is translated to a <br> by the format library.
On the RN side in iOS it looks we allow Aztec to proceed adding the enter by itself, and it then adds a <p> to the resulting content that makes this undesired result.

While this solution works I think that in the future we need to have the same behaviour in Aztec or else further problems will arise.

@etoledom
Copy link
Contributor Author

etoledom commented May 1, 2019

Thank you @SergioEstevao for checking this out!

We are in fact intercepting every Enter stroke and letting gutenberg manage it.

Here is where it's intercepted:
https://github.com/wordpress-mobile/gutenberg-mobile/blob/26bae633aa0d7661719bcb99395888f6301cc99e/react-native-aztec/ios/RNTAztecView/RCTAztecView.swift#L272

Here is where the line break is inserted (very similar to Web):

const insertedLineBreak = { needsSelectionUpdate: true, ...insert( currentRecord, '\n' ) };

I did further investigation to be sure, and here is a bit more clear what is happening:

This is the communication between RichText.js and RCTAztecView.swift with this fix in place, seen from the RCTAztecView.swift perspective:

-> HTML sent: <p>Hello</p>
-> Enter intercepted
-> HTML received: Hello<br>
-> HTML sent: <p>Hello<br></p>

Aztec by default wraps the content with <p> tags. When Enter gets intercepted, Aztec receives the HTML free of p tags, but it wraps them again.

Now this is the same without the fix:

-> HTML sent: <p>Hello</p>
-> Enter intercepted
-> HTML received: <p>Hello</p><br>
-> HTML sent: <p>Hello</p><p><br></p>

When Enter is intercepted, gutenberg insert the line break tag after the p tags added by Aztec, and then Aztec receives it, and does its job of wrapping this new HTML paragraph (the br) in p tags.

I'm not sure if we can safely deactivate that p wrapping behavior from Aztec, or if we do that, how will it affect other blocks. If that is the correct way, we should definitely investigate this further.

Please let me know what do you think 🙂

@SergioEstevao
Copy link
Contributor

@etoledom let's merge has it is, further down the line we can investigate on how to improve this.

@etoledom
Copy link
Contributor Author

etoledom commented May 2, 2019

Thank you 🙏

@etoledom etoledom merged commit 1e8739e into rnmobile/feature-caption May 2, 2019
@etoledom etoledom deleted the rnmobile/fix-ios-multi-paragraph-caption branch May 2, 2019 13:22
etoledom added a commit that referenced this pull request May 16, 2019
* [Mobile]: Add Image Caption Styling (#14883)

* Add ability to style image caption

Adds ability to style image caption with bold, italic, links, and strikethrough (wordpress-mobile/gutenberg-mobile#574).
Adds ability for image captions to wrap to multiple lines instead of getting
cut off (wordpress-mobile/gutenberg-mobile#590).

* Address style errors from linter

* Hide image toolbar controls if caption selected

* Update state in componentDidMount

The previous approach of setting the child component's `isSelected` prop
at the time the child was rendered based on
`this.props.isSelected && this.state.isCaptionSelected` did not work when:

  (a) the child was selected (so its `isSelected` prop was true);
  (b) the child component had no text; and
  (c) the parent's `isSelected` prop was changed to false
      (i.e., another block was selected).

Because the child component is not rendered when both the parent's `isSelected`
prop is false and the caption does not contain any text, the child
component's `onBlur` prop function (the `onCaptionBlur function) would not
be called and update the `isCaptionSelected` state to be false.

This bug shows when a user selects an empty caption, then selects a different
block (thereby hiding the caption since it is empty), and then re-selects the
image with the empty caption. In that scenario, upon re-selecting the image
with the empty caption, the empty caption would appear and immediately be
selected instead of just appearing. This bug would not appear if there
was any text in the caption, because then the caption would always
render and its `onBlur` prop function would be called.

* Add comment explaining isSelected guard

* Update image caption tagname from p to figcaption

This change brings mobile's handling of image captinos in line
with the web. It also addresses a crash that was occurring when
the enter key was tapped to enter a new line in an image caption.

* Remove `onBlurCaption` function

On iOS the display of the link UI modal was causing the `onBlurCaption`
function to be called, which would update the image caption's `isSelected`
prop to false. That would, in turn, immediately remove the
just-displayed modal. Restructuring the logic to not like this to not
use the image caption's `onBlur` function avoids that issue.

* Remove tagName from caption RichText component

Explicitly setting the tagName to figcaption caused an
invalid block to be saved if a caption ended with an empty line.

* iOS: Remove p tags from image caption paragraphs (#15366)

* Add center alignment to image caption on mobile (#15257)

* Adding missing style to video/style.native.scss
This style was fetched from the Image block, and it was removed after replacing the TextInput with RichText on captions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants